Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add :where() blog post #2240

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Add :where() blog post #2240

wants to merge 1 commit into from

Conversation

beerinho
Copy link
Contributor

This PR adds a new blog post about using data-selectors and the :where() pseudo-function to help style your base components to make them easier to override and easier to access.

Three things I'd especially like feedback on:

  • do you think the component images fit in? Would it be better to have them displayed in a different way?
  • does it matter which image format I used? At the moment the images are PNG but I noticed they get transformed anyway,
  • the "get in touch" section at the bottom feels repetitive as we already have 2 CTAs about Ember on the page

@beerinho beerinho requested a review from a team November 24, 2023 12:53
@beerinho beerinho self-assigned this Nov 24, 2023
Copy link
Member

@marcoow marcoow left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The post reads well but I don't really get what problem it solves and am not sure whether this doesn't actually introduce a new problem anyway…

element set at three different levels, meaning that we would need to be very
careful about the selectors we are using to make sure we are setting the styles
without doing anything that might be impossible to override if we wanted to
extend our `Card` component later on: I’m looking at you `!important`.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't understand the problem tbh. The Card component would set the padding for a class it defines with in its own scope and sets on the Button component?

<style>
.button {
  padding: 5px;
}
</style>

<template>
  <!-- this is the Card component's template -->
  <div><Button class="button" /></div>
</template>

I'm not sure how we need to be careful which selectors we use as the Card component can only use the .button selector anyway as it cannot even know what the class names in the <Button> component are?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess I need to find a better way to explain this, essentially the issue is that if the Button component has 2/3 selectors then the specificity will be higher and therefore we would need to be careful with the selectors in the Card component to make sure the base styles are being overridden

components directly - that is both the `PrimaryButton` and `Button` because they
are both using the same underlying element. This means that we can easily
override the styles of the base component without needing to drill down through
the whole component tree.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But this means that the Card component is now implicitly coupled to an implementation detail in the Button component (the fact that it sets the data-button attribute on the button it renders). I'd argue this is an anti-pattern and if you want to change the buttons, you should set a class?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you wanted to set the padding for all Buttons at any nesting level within the Card component, isn't that what CSS variables are for?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had the issue of trying to make this example as simple as possible so that the focus could be the thing that I wanted to showcase but I think it is too simple in this case. The main issue that I am failing to point out is that you might not have a way to add a class to that component directly; if it is 3+ components deep then you wouldn't want to have to pass a class all the way through those components. So in this scenario I can see why this doesn't make sense and I need to think of a better way to showcase it.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But still, isn't setting the style for a component 3 levels down the tree based on a [data-] selector an anti-pattern since you're specifically using that technique to bypass CSS encapsulation? Isn't this what CSS variables are for so that the component can be explicit about what style properties are supposed to be overridden (and thus part of its public API)?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We definitely could use a CSS variable here, and I think that's another flaw with this example, because overriding padding is easily done in that way, but if we wanted to override multiple things or if we needed to drill down deeper to get to the property then it wouldn't work as well.

I'll add in the example with css variables and then explain the benefits and drawbacks

the `background-color` remains unchanged.

As an app begins to grow, and multiple components start to be reused in
different scenarios, minor changes can start to have a big impact across the app
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

minor changes like not setting data-button anymore in the Button component :)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In this scenario the data-button attribute would be one of the foundational blocks suggested, so it would have big consequences to remove it for sure. But you could put it on par with the class: the button component would definitely not look as intended without it

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, the button would look wrong without the data-button attribute but that's not the question (you can imagine a refactoring where the attribute is renamed and the component's own styles are changed accordingly). The issue here is that the data-button attribute becomes part of the component's public API and all of the component's own/internal styles become part of it's API as well in a way since you're allowing all of the styles to be changed via selectors based on the data-button attribute – whatever you change, you'll need to make sure it's consistent with other styles (e.g. aspect ratios or whatever; if you're exposing a CSS variable you can allow that to be changed and then define styles that depend on the value of the variable, thus enforcing consistency like width is always twice the height or whatever).

In short, what I'm saying is I have the feeling that this post recommends a pattern, I'd consider an anti-pattern actually – of course I might still be misunderstanding sth.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you touch on an interesting point about what should be considered the component's public API. I'm thinking I've just framed it wrong, perhaps this isn't a suggestion for how to build the foundation of your app, but rather a way to expose the ability to update styles in a mature app without having to refactor everything

data-loading={{@loading}}
local-class="button"
type="button"
{{on "click" this.onClick}}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would change the indent with 2 spaces here.

@marcoow
Copy link
Member

marcoow commented Jun 5, 2024

@beerinho what's the status of this?

imageAlt:
---

Foreword: Should out to
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
Foreword: Should out to
Foreword: Shout out to


Foreword: Should out to
[Florian Pichler](https://mainmatter.com/blog/author/pichfl) for introducing me
to this pattern earlier this year.
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
to this pattern earlier this year.
to this pattern recently.

@beerinho
Copy link
Contributor Author

beerinho commented Jun 21, 2024

@beerinho what's the status of this?

I haven't had a chance to revisit this in a meaningful way. And since the feedback you gave - the only feedback - seems to point towards it being an anti-pattern, I'm wondering if it should be a post at all. The other option is to reframe it and either remove the usage of data- attributes or make it clearer that the data- attribute can be used as an escape hatch rather than a useful way of styling; or perhaps using global classes instead of data- attributes would be better for this, but that still comes with the issue of it being part of the component's public API.

This post also makes extensive use of ember-css-modules which isn't embroider friendly, which already puts a shelf-life on it.

@marcoow
Copy link
Member

marcoow commented Jun 21, 2024

And since the feedback you gave - the only feedback - seems to point towards it being an anti-pattern

I mean I could totally be wrong about this of course :) – happy to exchange thoughts and come to a conclusion

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants